Ft/build account switcher panel#168
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
💤 Files with no reviewable changes (2)
📝 WalkthroughWalkthroughAdds account-switching functionality: a DB query ( ChangesAccount switching feature
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant DraftingScreen
participant useActiveAccountSummary
participant DraftingHeader
participant AccountInitialsButton
participant AccountSwitcherPanel
participant useDeviceAccounts
participant getUserById
DraftingScreen->>useActiveAccountSummary: fetch summary keyed by refreshKey
useActiveAccountSummary-->>DraftingScreen: account details
DraftingScreen->>DraftingHeader: renderHeader() with account props
DraftingHeader->>AccountInitialsButton: showAccountIndicator, onAccountPress
AccountInitialsButton-->>DraftingScreen: onPress opens switcher
DraftingScreen->>AccountSwitcherPanel: renderAccountSwitcher() visible=true
AccountSwitcherPanel->>useDeviceAccounts: visible=true triggers reload()
useDeviceAccounts->>getUserById: fetch user by known userId
getUserById-->>useDeviceAccounts: DBTypes.User or null
useDeviceAccounts-->>AccountSwitcherPanel: accounts, hasAccountLimit, loading
AccountSwitcherPanel-->>DraftingScreen: onClose closes panel
Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
src/utils/accountDisplay.ts (1)
1-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding unit tests for the initials/display-name branching logic.
Several edge cases (missing names, whitespace-only names, single vs. multi-part email local-parts) are handled correctly, but the branching is non-trivial and easy to regress silently.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/accountDisplay.ts` around lines 1 - 45, Add unit tests for the branching in getAccountDisplayName and getAccountInitials in accountDisplay.ts, covering missing names, whitespace-only first/last names, email fallback behavior, and both single-part and multi-part email local-parts. Use the public helpers getAccountDisplayName, getAccountInitials, and the internal behaviors around buildEmailFallback/firstCharacter as the cases to verify so regressions in these edge paths are caught.src/app/screens/DraftingScreen.tsx (1)
22-24: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove leftover commented-out code.
// useDraftingContext,and{/* <DraftingPlayerBar verses={verses} /> */}are dead code left in from the refactor.🧹 Cleanup
import { DraftingProvider, - // useDraftingContext,- {/* <DraftingPlayerBar verses={verses} /> */} -Also applies to: 200-200
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/screens/DraftingScreen.tsx` around lines 22 - 24, Remove the leftover commented-out code from the Drafting screen refactor. In DraftingScreen, delete the unused commented import `useDraftingContext` and the commented JSX for `DraftingPlayerBar`, keeping only the active imports and rendered components. Use the identifiers DraftingScreen, DraftingProvider, useDraftingContext, and DraftingPlayerBar to locate the dead code and clean it out.src/components/ui/AccountSwitcherPanel.tsx (2)
47-53: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBackdrop
Pressablemay swallow taps on non-interactive sheet areas.The sheet body (Line 48) is a plain
Viewnested inside the backdropPressable. Taps on padding/gaps within the sheet (not on aTouchableOpacity) will likely bubble up and triggeronClose, closing the modal unexpectedly.🔧 Proposed fix to stop propagation on the sheet
- <View + <Pressable + onPress={() => {}} style={[ styles.sheet, { paddingBottom: insets.bottom + theme.spacing.lg }, ]} > ... - </View> + </Pressable>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/ui/AccountSwitcherPanel.tsx` around lines 47 - 53, The backdrop Pressable in AccountSwitcherPanel is wrapping the entire sheet, so taps on non-interactive areas inside the sheet can still trigger onClose. Move the close handler to the backdrop only and stop touch propagation for the sheet container itself, using the existing Pressable/View structure around styles.backdrop and styles.sheet so taps inside the sheet body do not dismiss the modal unexpectedly.
160-161: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHardcoded hex/rgba colors alongside theme tokens.
'rgba(10, 18, 40, 0.44)','#F3F7FF','#2E5BD8','#F1F4FA'are hardcoded even though the rest of the stylesheet consistently usestheme.colors.*. Consider adding equivalent theme tokens for consistency and easier future theming (e.g., dark mode).Also applies to: 208-211, 222-223, 266-267
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/ui/AccountSwitcherPanel.tsx` around lines 160 - 161, Replace the remaining hardcoded color values in AccountSwitcherPanel’s stylesheet with theme-backed values to match the existing `theme.colors.*` pattern. Update the styles around the background and text/border states referenced in `AccountSwitcherPanel` (including the selection, hover, and divider-related styles) so they use equivalent tokens instead of `rgba(10, 18, 40, 0.44)`, `#F3F7FF`, `#2E5BD8`, and `#F1F4FA`. If matching tokens do not exist yet, add them to the theme and then consume them here.src/hooks/useActiveAccountSummary.ts (1)
25-41: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated cancellation-guard boilerplate across hooks.
The
mountedRef/requestIdRefsetup and cleanup effect (Lines 29-37) is duplicated almost verbatim insrc/hooks/useDeviceAccounts.ts(Lines 33-41). Consider extracting a small shared hook (e.g.useCancellableAsync()returning{ mountedRef, nextRequestId }) to keep the guard logic consistent and avoid maintaining it in two places.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks/useActiveAccountSummary.ts` around lines 25 - 41, The mountedRef/requestIdRef cancellation guard in useActiveAccountSummary is duplicated in useDeviceAccounts, so extract the shared lifecycle/request tracking into a small reusable hook such as useCancellableAsync that exposes the mounted state and a nextRequestId helper. Then update useActiveAccountSummary and useDeviceAccounts to consume that shared hook instead of each defining their own refs and cleanup effect, keeping the cancellation logic centralized and consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/app/screens/DraftingScreen.tsx`:
- Around line 80-105: The account switch flow is wired to pass onUserSwitched
into AccountSwitcherPanel, but the row taps inside that component never invoke
it, so selecting an account does nothing. Update the AccountSwitcherPanel
account-row press handling to call the provided onUserSwitched callback when a
user row is tapped, and ensure DraftingScreen’s handleUserSwitched is reached
through that path. Keep the existing close behavior intact while adding the
missing tap handler on the account item component(s).
In `@src/components/ui/AccountSwitcherPanel.tsx`:
- Around line 21-30: The account rows in AccountSwitcherPanel are currently
non-interactive because onUserSwitched is never destructured or used, and the
row UI has no press handler. Update the AccountSwitcherPanel component to accept
onUserSwitched alongside visible and onClose, wire each account row’s onPress to
the existing switchActiveUser flow, and invoke onUserSwitched after a successful
switch so the parent DraftingScreen can refresh state.
---
Nitpick comments:
In `@src/app/screens/DraftingScreen.tsx`:
- Around line 22-24: Remove the leftover commented-out code from the Drafting
screen refactor. In DraftingScreen, delete the unused commented import
`useDraftingContext` and the commented JSX for `DraftingPlayerBar`, keeping only
the active imports and rendered components. Use the identifiers DraftingScreen,
DraftingProvider, useDraftingContext, and DraftingPlayerBar to locate the dead
code and clean it out.
In `@src/components/ui/AccountSwitcherPanel.tsx`:
- Around line 47-53: The backdrop Pressable in AccountSwitcherPanel is wrapping
the entire sheet, so taps on non-interactive areas inside the sheet can still
trigger onClose. Move the close handler to the backdrop only and stop touch
propagation for the sheet container itself, using the existing Pressable/View
structure around styles.backdrop and styles.sheet so taps inside the sheet body
do not dismiss the modal unexpectedly.
- Around line 160-161: Replace the remaining hardcoded color values in
AccountSwitcherPanel’s stylesheet with theme-backed values to match the existing
`theme.colors.*` pattern. Update the styles around the background and
text/border states referenced in `AccountSwitcherPanel` (including the
selection, hover, and divider-related styles) so they use equivalent tokens
instead of `rgba(10, 18, 40, 0.44)`, `#F3F7FF`, `#2E5BD8`, and `#F1F4FA`. If
matching tokens do not exist yet, add them to the theme and then consume them
here.
In `@src/hooks/useActiveAccountSummary.ts`:
- Around line 25-41: The mountedRef/requestIdRef cancellation guard in
useActiveAccountSummary is duplicated in useDeviceAccounts, so extract the
shared lifecycle/request tracking into a small reusable hook such as
useCancellableAsync that exposes the mounted state and a nextRequestId helper.
Then update useActiveAccountSummary and useDeviceAccounts to consume that shared
hook instead of each defining their own refs and cleanup effect, keeping the
cancellation logic centralized and consistent.
In `@src/utils/accountDisplay.ts`:
- Around line 1-45: Add unit tests for the branching in getAccountDisplayName
and getAccountInitials in accountDisplay.ts, covering missing names,
whitespace-only first/last names, email fallback behavior, and both single-part
and multi-part email local-parts. Use the public helpers getAccountDisplayName,
getAccountInitials, and the internal behaviors around
buildEmailFallback/firstCharacter as the cases to verify so regressions in these
edge paths are caught.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 941218ea-d516-4247-b8c6-35736400c153
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
package.jsonsrc/app/screens/DraftingScreen.tsxsrc/components/layout/DraftingHeader.tsxsrc/components/ui/AccountInitialsButton.tsxsrc/components/ui/AccountSwitcherPanel.tsxsrc/db/queries.tssrc/hooks/useActiveAccountSummary.tssrc/hooks/useDeviceAccounts.tssrc/types/db/types.tssrc/utils/accountDisplay.ts
Screenshots:

